Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options for ConfigFactory to handle customConfig properly #2588

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

mkondratek
Copy link
Contributor

@mkondratek mkondratek commented Nov 6, 2024

Fixes https://linear.app/sourcegraph/issue/CODY-4264/a-comment-in-cody-settingsjson-breaks-cody-initialization-stuck-at.

Test plan

  1. Have the cody_settings.json like this:
{
  // other providers...
  "openctx.providers": {
    "https://gist.githubusercontent.com/thenamankumar/3708f084fb2abd57adafe7f14620bdf7/raw/e7c7059cb5b04f6feead002e7b3a90c5b1a4bb96/provider.js": true
  }
}
  1. Cody should initialize

@mkondratek mkondratek self-assigned this Nov 6, 2024
Comment on lines 144 to 142
val config = ConfigFactory.parseString(text).resolve()
val config = JsonParser.parseString(text).asJsonObject
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigFactory does not render json. The rendered config contained the comment with # instead of //. Using JsonParser fixes the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other option would be adding .setComments(false) in the current code.
I thought Lightened Config was a bit more forgiving when parsing e.g. additional commas.
Maybe we can write simple test for that and check how those corner cases looks in case of both approaches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@mkondratek mkondratek force-pushed the mkondratek/fix/customConfigComments branch from 95ac480 to 6264807 Compare November 7, 2024 09:37
@mkondratek mkondratek changed the title Use JsonParser instead of ConfigFactory for customConfig Add options for ConfigFactory to handle customConfig properly Nov 7, 2024
additionalProperties.forEach { (key, value) ->
config.withValue(key, ConfigValueFactory.fromAnyRef(value))
Copy link
Contributor Author

@mkondratek mkondratek Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug 😄 configs are immutable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, we ignored "foldingRanges": "indentation-based" - how important for us is this option?

Comment on lines +27 to +33
parsed
.get("cody")
.asJsonObject
.get("experimental")
.asJsonObject
.get("foldingRanges")
.asString)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withValue adds the field as

"cody": {
  "experimental": {
    "foldingRanges": "indentation-based"
  }
}

instead of

"cody.experimental.foldingRanges": "indentation-based"

Is it a problem? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO It is not, those are equivalent in VSC config.

Copy link
Contributor

@pkukielka pkukielka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkondratek mkondratek merged commit 6ab342f into main Nov 7, 2024
9 of 10 checks passed
@mkondratek mkondratek deleted the mkondratek/fix/customConfigComments branch November 7, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants